-
-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable diskless replication configuration #340
enable diskless replication configuration #340
Conversation
@brianbianco merge pls. |
@shortdudey123 Could you review this? It looks good to me. Has a guard for the redis 2.8+ only features in the config template and adds the options for them. |
templates/default/redis.conf.erb
Outdated
@@ -250,6 +250,69 @@ dir <%=@datadir%> | |||
# but to INFO and SLAVEOF. | |||
# | |||
slave-serve-stale-data <%=@slaveservestaledata%> | |||
<% if @version[:major].to_i == 2 && @version[:minor].to_i >= 6 %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional logic doesn't seem match the comment
# Since Redis 2.6 by default slaves are read-only
Edit: Actually the conditional does match, but the code block would indicate this should only be added to the config file for Redis < 2.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is 2.6+, the if statement should be adjusted as such
templates/default/redis.conf.erb
Outdated
# administrative / dangerous commands. | ||
slave-read-only <%=@slavereadonly%> | ||
<% end %> | ||
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or @version[:major].to_i > 2 =%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't match a version string of 2.9.1
. I'm pretty sure they're done with normal releases in the 2.x series, but possible security releases may bump minor versions in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same w/ this if
templates/default/redis.conf.erb
Outdated
# administrative / dangerous commands. | ||
slave-read-only <%=@slavereadonly%> | ||
<% end %> | ||
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or @version[:major].to_i > 2 =%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same w/ this if
b1015e3
to
06a7762
Compare
@shortdudey123 fixed both version checks. |
06a7762
to
24239ce
Compare
templates/default/redis.conf.erb
Outdated
@@ -250,6 +250,69 @@ dir <%=@datadir%> | |||
# but to INFO and SLAVEOF. | |||
# | |||
slave-serve-stale-data <%=@slaveservestaledata%> | |||
<% if @version[:major].to_i == 2 && @version[:minor].to_i >= 6 || @version[:major].to_i == 3 %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd change it to look like this so it works against Redis 4+
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 6) || @version[:major].to_i >= 3 -%>
<% ######## Redis 2.6 and higher ######## -%>
templates/default/redis.conf.erb
Outdated
# administrative / dangerous commands. | ||
slave-read-only <%=@slavereadonly%> | ||
<% end %> | ||
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i > 2 && @version[:minor].to_i >= 9 ) =%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file didn't pass a syntax test (erb -x -T '-' redis.conf.erb | ruby -c
) because of the =%>
close tag. ERB / ruby syntax testing would probably be helpful, though I'm not yet familiar w/ the current testing methods in this cookbook to figure out how to add it.
I'd change this line to look like
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%>
<% ######## Redis 2.8.19 and higher ######## -%>
24239ce
to
4d2357d
Compare
@devsibwarra thanks for suggestions, fixed. |
templates/default/redis.conf.erb
Outdated
<% end %> | ||
<% if (@version[:major].to_i == 2 && @version[:minor].to_i >= 8 && @version[:tiny].to_i > 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%> | ||
<% ######## Redis 2.8.19 and higher ######## -%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed your comment under the README says the repldisklesssync
option Requires redis 2.8.18+
. Update the tiny version operator here to include that version and tweak the comment accordingly.
<% if (@version[:major].to_i == 2 && @version[:minor].to_i == 8 && @version[:tiny].to_i >= 18) or (@version[:major].to_i == 2 && @version[:minor].to_i >= 9) or @version[:major].to_i >= 3 -%>
<% ######## Redis 2.8.18 and higher ######## -%>
Note: I also tweaked the minor version from >= 8
to == 8
to properly match versions. It should be fine in the old method, but having it more precise should avoid confusion
4d2357d
to
7b7d8fc
Compare
@devsibwarra done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@shortdudey123 i think we need bump cookbook version too? |
A new release will be done once i can churn through more of https://github.com/brianbianco/redisio/milestone/12 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Rebased #235